Repository navigation
Improve SSL performance by avoiding SSLWantReadError exception and using much faster checks whenever possible - #629
Conversation
tarasko
changed the title
tarasko
changed the title
|
Can you expend on how you figured out it handles 99% of cases, and what are the last 1%? |
|
99% - 1% is figurative. I don't know the real numbers and obviously it depends on a particular use case. Checking In my use case client and server exchange a lot of small messages and every message (every SSL frame) can fit into a single TCP packet. Incoming data can also fit into all receiving buffers. So it never happens that uvloop reads only a part of SSL frame, it is always a complete frame, and SSLObject.read is able to process all incoming data. Checking |
fantix
left a comment
fantix
left a comment
There was a problem hiding this comment.
This is a good one!
Though, it's trickier to verify correctness than just SSLWantReadError, because SSLObject.read() may also raise SSLWantWriteError during close_notify and re-negotiation. The two points to verify are:
- If
SSLObject.read()should be called regardlessly per_do_read(). - If
SSLObject.read()should be called regardlessly per loop within_do_read().
(1) is triggered by I/O read event or user calling transport.resume_reading(). In such scenarios, I don't see a case where the SSLObject has a pending state that requires read() to handle, if both buffers are empty.
(2) is where this optimization happens. It challenges the SSLObject state machine assumption, which is, to call read() until it returns 0, or raises an error. In other words, if a first read() call didn't return 0 or raise an error, can we safely skip the next read() call if both buffers are empty? Reading the underlying C code, I don't see any state left behind. And our use case here (sslproto.pyx) doesn't have other changes to the SSLObject state during _do_read(). Also, given that our sophisticated test cases are passing, I believe it's safe to apply this optimization.
| else: | ||
|
|
||
| last_bytes_read = <Py_ssize_t>self._sslobj_read( | ||
| app_buffer_size, app_buffer) |
There was a problem hiding this comment.
| app_buffer_size, app_buffer) | |
| app_buffer_size - total_bytes_read, app_buffer) |
There was a problem hiding this comment.
That's a good one. Fixed!
There was a problem hiding this comment.
Actually, probably doesn't make a difference because SSLObject.read won't read more than the length of app_buffer and it is already app_buffer_size - total_bytes_read. I fixed it anyway for consistency
There was a problem hiding this comment.
Yes, this whole SSLWantWriteError from SSLObject.read is tricky.
I'm not 100% sure but I have the following assumption:
-
Python ssl.MemoryBIO grows dynamically without fixed capacity, has no limit, write can only fail in case of system out-of-memory
-
SSL_read_ex returns SSL_WANT_WRITE_ERROR when memory BIO runs out of memory, not when we just wrote something. So given (1) SSL_WANT_WRITE_ERROR may only happen when the system runs out of memory.
-
SSL_read_ex does use its chance to write control stuff right after successful read before returning. link
| if app_buffer_size == 0: | ||
| return |
There was a problem hiding this comment.
Ahh, good catch! This fixes a false EOF bug in previous code I think. Would be nice to have a test!
Changes ======= * Add Python 3.15 and 3.15t wheel builds and CI coverage (MagicStack#758) (by @honglei @fantix in f7c0547) * Add support for the `eager_start` keyword argument in create_task() (MagicStack#748) (by @samypr100 in 3cbb095 for MagicStack#746 MagicStack#718) * Add thread name prefix to the default thread pool executor (MagicStack#636) (by @inikolaev @fantix in 0582f94 for MagicStack#562) * Add support for special hostname `<broadcast>` (MagicStack#592) (by @jpbede in 3060ceb for MagicStack#540) * Upgrade libuv to v1.52.1 (MagicStack#753) (by @fantix in e8efea4 for MagicStack#752) * Improve performance by using Python C API to enter/exit context (MagicStack#627) (by @tarasko in 837ef22) * Improve performance/latency of Transport.write (MagicStack#619) (by @tarasko in 1d9b6e0) * Replace some SSL vectorcall with direct methods (MagicStack#626) (by @tarasko in 6a27cbe) * Optimize SSL buffered reads using C values (MagicStack#629) (by @tarasko in a308f75) Fixes ===== * Detach socket on create_connection cancellation to prevent fd double-close (MagicStack#740) (by @junjzhang @fantix in dc680eb for MagicStack#645 MagicStack#738) * Remove `loop._ready_len` in favor of `len(loop._ready)` (MagicStack#721) (by @x42005e1f in 5910a18 for MagicStack#720) * Prefer inspect.iscoroutinefunction (MagicStack#705) (by @MatthieuDartiailh in fd65027 for MagicStack#703) * Fix context tests by explicitly yielding after run_in_executor (MagicStack#743) (by @samypr100 in 6cd24cb) * Fix test_create_connection_open_con_addr with Python 3.13.9+ (MagicStack#713) (by @shadchin in b93141a for MagicStack#701) * Skip flaky test_cancel_post_init on asyncio 3.13+ (MagicStack#714) (by @fantix in 3ea5c85 for MagicStack#709) * Fix flaky test_fs_event (MagicStack#717) (by @fantix in 8da4547) * Use C __atomic builtins for debug counters (MagicStack#719) (by @fantix in 836e3b2) * Update the example to work with Python 3.14 (MagicStack#710) (by @Jamie-Chang in b74c2f1) Build ===== * Replace pkg_resources with packaging and use Cython 3.1 (MagicStack#742) (by @samypr100 in b377b7c for MagicStack#729) * Remove wheel as a build dependency (MagicStack#696) (by @DimitriPapadopoulos in 173e88c) * Support any number of flags in UVLOOP_OPT_CFLAGS (MagicStack#630) (by @mgorny in 963a5f3)
3 participants
SSLWantReadError is expensive.
python/cpython#123954
This PR tries to predict that there will be SSLWantReadError by checking incoming.pending and SSLObject.pending() first.
This check works in 99% of cases. For the rest 1% we still rely on SSLWantReadError